chore: update daemonset to use resource processor#46
chore: update daemonset to use resource processor#46sleepyfoodie wants to merge 3 commits intomainfrom
Conversation
|
|
||
| CLUSTER_NAME="${CLUSTER_NAME:-queries-testing}" | ||
| KWOK_CONTEXT="kwok-${CLUSTER_NAME}" | ||
| KWOK_NET="kwok-${CLUSTER_NAME}" |
There was a problem hiding this comment.
Pull request overview
This PR updates the OpenTelemetry collector daemonset configuration to use the resource processor instead of the attributes processor for setting the k8s.cluster.name attribute. Additionally, it includes dependency updates for the kwok-stats-proxy module, a Go version upgrade, and improvements to the cleanup scripts.
Changes:
- Updated OpenTelemetry processor from
attributes/k8sclusternametoresource/k8sclusternamein the daemonset configuration - Updated Go dependencies (golang.org/x/net, gopkg.in/evanphx/json-patch.v4, k8s.io/kube-openapi, go.yaml.in/yaml/v2)
- Upgraded base image from golang:1.25 to golang:1.26 in kwok-stats-proxy Dockerfile
- Enhanced cleanup scripts with network-based container removal and additional container names
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/otel-collector-daemonset.values.yaml | Changed processor type from attributes to resource for k8s.cluster.name |
| scripts/kwok-stats-proxy/go.mod | Updated Go version to 1.26.0 and removed unused pkg/errors dependency |
| scripts/kwok-stats-proxy/go.sum | Updated dependency checksums for version bumps |
| scripts/kwok-stats-proxy/Dockerfile | Upgraded golang base image to 1.26 and removed SHA256 digest pin |
| scripts/run-kwok-env.sh | Added KWOK_NET variable for consistent network naming |
| Makefile | Enhanced cleanup to include kwok-otel-gateway and network-based container removal |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| resource/k8sclustername: | ||
| attributes: | ||
| - key: k8s.cluster.name | ||
| action: upsert | ||
| value: kubernetes-mixin-otel |
There was a problem hiding this comment.
The processor name has been updated from attributes/k8sclustername to resource/k8sclustername in this file, but the same processor in scripts/kwok-config/kwok-hostmetrics-replicator.yaml (lines 35-39 and 61) still uses the old attributes/k8sclustername name. This inconsistency could cause configuration errors. Both files should use the same processor type for setting the cluster name attribute.
| @@ -1,4 +1,4 @@ | |||
| FROM golang:1.25-alpine@sha256:98e6cffc31ccc44c7c15d83df1d69891efee8115a5bb7ede2bf30a38af3e3c92 AS builder | |||
| FROM golang:1.26-alpine AS builder | |||
There was a problem hiding this comment.
The SHA256 digest pin has been removed from the FROM instruction. This makes the build less reproducible and could introduce supply chain security risks, as the image tag could potentially point to different content over time. Consider keeping the digest pin for better security and reproducibility, especially for production builds.
| FROM golang:1.26-alpine AS builder | |
| FROM golang:1.26-alpine@sha256:<exact-digest-for-golang-1.26-alpine> AS builder |
| @docker rm -f kwok-otel-collector kwok-stats-proxy kwok-beyla lgtm 2>/dev/null || true | ||
| @for c in $$(docker ps -a -q --filter "name=kwok-hostmetrics-replicator" 2>/dev/null); do docker rm -f $$c 2>/dev/null || true; done | ||
| @docker rm -f kwok-otel-collector kwok-otel-gateway kwok-stats-proxy kwok-beyla lgtm 2>/dev/null || true | ||
| @for c in $$(docker ps -a -q --filter "name=kwok-hostmetrics" 2>/dev/null); do docker rm -f $$c 2>/dev/null || true; done |
There was a problem hiding this comment.
The filter pattern has been changed from "name=kwok-hostmetrics-replicator" to "name=kwok-hostmetrics". This is a broader filter that will match any container name starting with "kwok-hostmetrics", including "kwok-hostmetrics-replicator" and potentially other containers. Ensure this is intentional and that you want to clean up all containers matching this pattern, not just the replicator containers.
| @for c in $$(docker ps -a -q --filter "name=kwok-hostmetrics" 2>/dev/null); do docker rm -f $$c 2>/dev/null || true; done | |
| @for c in $$(docker ps -a -q --filter "name=kwok-hostmetrics-replicator" 2>/dev/null); do docker rm -f $$c 2>/dev/null || true; done |
| kwok-down: | ||
| @docker rm -f kwok-otel-collector kwok-stats-proxy kwok-beyla lgtm 2>/dev/null || true | ||
| @for c in $$(docker ps -a -q --filter "name=kwok-hostmetrics-replicator" 2>/dev/null); do docker rm -f $$c 2>/dev/null || true; done | ||
| @docker rm -f kwok-otel-collector kwok-otel-gateway kwok-stats-proxy kwok-beyla lgtm 2>/dev/null || true |
There was a problem hiding this comment.
The cleanup now includes removal of a 'kwok-otel-gateway' container, but there are no references to this container being created in any of the scripts in the repository. If this container is created elsewhere or is planned for future use, this is fine. Otherwise, this cleanup step may be unnecessary.
| @docker rm -f kwok-otel-collector kwok-otel-gateway kwok-stats-proxy kwok-beyla lgtm 2>/dev/null || true | |
| @docker rm -f kwok-otel-collector kwok-stats-proxy kwok-beyla lgtm 2>/dev/null || true |
skl
left a comment
skl
left a comment
There was a problem hiding this comment.
@sleepyfoodie please check the copilot suggestions, they seem reasonable. Feel free to ping me when ready.
No description provided.